Close files opened by hack helper commands#4709
Conversation
|
Hi @buley. Thanks for your PR. I'm waiting for a Azure member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #4708 by ensuring short-lived helper commands under hack/ properly close any files they open during read/parse operations, avoiding unnecessary file descriptor leaks.
Changes:
- Add a deferred
Close()for the manifest file opened byhack/role. - Add best-effort closing for files opened by
hack/aeadwhen-fileis not-(stdin remains untouched).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| hack/role/role.go | Defer-close the manifest file after opening it for parsing. |
| hack/aead/aead.go | Track and defer-close the opened input file when reading from a path instead of stdin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
buley
requested review from
alcasim,
bennerv,
cadenmarchese,
hawkowl,
hlipsig,
jharrington22,
kimorris27,
mociarain,
mrWinston,
rogbas,
sankur-codes,
tiguelu,
tsatam,
tuxerrante,
ventifus,
wanghaoran1988 and
yjst2012
as code owners
|
Addressed the Copilot feedback in |
Which issue this PR addresses:
Fixes #4708
What this PR does / why we need it:
This closes the files opened by
hack/aeadandhack/roleonce their read and parse paths complete. Thehack/aeadchange keeps the existing stdin path untouched while cleaning up only real file opens.Test plan for issue:
go test ./hack/aead ./hack/roleIs there any documentation that needs to be updated for this PR?
No. This is an internal resource-cleanup change in
hack/.How do you know this will function as expected in production?
The change only adds file cleanup after existing reads complete and does not alter CLI flags, parsing logic, or the data produced by either helper.